Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tls: avoid taking ownership of OpenSSL objects #53436

Merged
merged 1 commit into from
Jun 14, 2024
Merged

tls: avoid taking ownership of OpenSSL objects #53436

merged 1 commit into from
Jun 14, 2024

Conversation

tniessen
Copy link
Member

It is often unnecessary to obtain (shared) ownership of OpenSSL objects in this code, and it generally is more costly to do so as opposed to just obtaining a pointer to the respective OpenSSL object. Therefore, this patch replaces various OpenSSL function calls that take ownership with ones that do not.

@tniessen tniessen added tls Issues and PRs related to the tls subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. openssl Issues and PRs related to the OpenSSL dependency. labels Jun 12, 2024
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Jun 12, 2024
@anonrig
Copy link
Member

anonrig commented Jun 12, 2024

Is there a noticable performance impact of this pull-request that you know of?

@tniessen
Copy link
Member Author

@anonrig None that I'd care to measure. Most of OpenSSL's shared ownership management is just based on reference counting so it barely has any performance impact.

@tniessen tniessen added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jun 13, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 13, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@tniessen
Copy link
Member Author

Looks like there are some incompatible definitions in OpenSSL 1.1.1...

@tniessen tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 13, 2024
@tniessen
tls: avoid taking ownership of OpenSSL objects
f974aaf 
It is often unnecessary to obtain (shared) ownership of OpenSSL objects
in this code, and it generally is more costly to do so as opposed to
just obtaining a pointer to the respective OpenSSL object. Therefore,
this patch replaces various OpenSSL function calls that take ownership
with ones that do not.
@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 13, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 13, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59782/

@tniessen
Copy link
Member Author

Should be fixed. PTAL.

@tniessen tniessen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jun 14, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 14, 2024
@nodejs-github-bot nodejs-github-bot merged commit d302537 into nodejs:main Jun 14, 2024
57 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in d302537

tniessen added a commit to tniessen/node that referenced this pull request Jun 14, 2024
@tniessen
crypto: avoid taking ownership of OpenSSL objects
ac9c352 
It is often unnecessary to obtain (shared) ownership of OpenSSL objects
in this code, and it generally is more costly to do so as opposed to
just obtaining a pointer to the respective OpenSSL object. Therefore,
this patch replaces various OpenSSL function calls that take ownership
with ones that do not.

Refs: nodejs#53436
@tniessen tniessen mentioned this pull request Jun 14, 2024
Merged
targos pushed a commit that referenced this pull request Jun 20, 2024
@tniessen @targos
tls: avoid taking ownership of OpenSSL objects
4b47f89 
It is often unnecessary to obtain (shared) ownership of OpenSSL objects
in this code, and it generally is more costly to do so as opposed to
just obtaining a pointer to the respective OpenSSL object. Therefore,
this patch replaces various OpenSSL function calls that take ownership
with ones that do not.

PR-URL: #53436
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: James M Snell <jasnell@gmail.com>
sophoniie pushed a commit to sophoniie/node that referenced this pull request Jun 20, 2024
@tniessen @sophoniie
tls: avoid taking ownership of OpenSSL objects
d9e8ceb 
It is often unnecessary to obtain (shared) ownership of OpenSSL objects
in this code, and it generally is more costly to do so as opposed to
just obtaining a pointer to the respective OpenSSL object. Therefore,
this patch replaces various OpenSSL function calls that take ownership
with ones that do not.

PR-URL: nodejs#53436
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: James M Snell <jasnell@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@tniessen @bmeck
tls: avoid taking ownership of OpenSSL objects
4f88932 
It is often unnecessary to obtain (shared) ownership of OpenSSL objects
in this code, and it generally is more costly to do so as opposed to
just obtaining a pointer to the respective OpenSSL object. Therefore,
this patch replaces various OpenSSL function calls that take ownership
with ones that do not.

PR-URL: nodejs#53436
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Jun 23, 2024
@tniessen
crypto: avoid taking ownership of OpenSSL objects
296ce1b 
It is often unnecessary to obtain (shared) ownership of OpenSSL objects
in this code, and it generally is more costly to do so as opposed to
just obtaining a pointer to the respective OpenSSL object. Therefore,
this patch replaces various OpenSSL function calls that take ownership
with ones that do not.

Refs: #53436
PR-URL: #53460
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@targos targos mentioned this pull request Jun 25, 2024
Merged
@Karliz24 Karliz24 mentioned this pull request Jul 9, 2024
Open
aduh95 pushed a commit that referenced this pull request Jul 12, 2024
@tniessen @aduh95
crypto: avoid taking ownership of OpenSSL objects
fc37264 
It is often unnecessary to obtain (shared) ownership of OpenSSL objects
in this code, and it generally is more costly to do so as opposed to
just obtaining a pointer to the respective OpenSSL object. Therefore,
this patch replaces various OpenSSL function calls that take ownership
with ones that do not.

Refs: #53436
PR-URL: #53460
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
aduh95 pushed a commit that referenced this pull request Jul 16, 2024
@tniessen @aduh95
crypto: avoid taking ownership of OpenSSL objects
f82e20f 
It is often unnecessary to obtain (shared) ownership of OpenSSL objects
in this code, and it generally is more costly to do so as opposed to
just obtaining a pointer to the respective OpenSSL object. Therefore,
this patch replaces various OpenSSL function calls that take ownership
with ones that do not.

Refs: #53436
PR-URL: #53460
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
@tniessen @marco-ippolito
tls: avoid taking ownership of OpenSSL objects
d3330e7 
It is often unnecessary to obtain (shared) ownership of OpenSSL objects
in this code, and it generally is more costly to do so as opposed to
just obtaining a pointer to the respective OpenSSL object. Therefore,
this patch replaces various OpenSSL function calls that take ownership
with ones that do not.

PR-URL: #53436
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: James M Snell <jasnell@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
@tniessen @marco-ippolito
tls: avoid taking ownership of OpenSSL objects
2aceed4 
It is often unnecessary to obtain (shared) ownership of OpenSSL objects
in this code, and it generally is more costly to do so as opposed to
just obtaining a pointer to the respective OpenSSL object. Therefore,
this patch replaces various OpenSSL function calls that take ownership
with ones that do not.

PR-URL: #53436
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: James M Snell <jasnell@gmail.com>
@marco-ippolito marco-ippolito mentioned this pull request Jul 19, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Aug 19, 2024
@tniessen @marco-ippolito
crypto: avoid taking ownership of OpenSSL objects
f0bc694 
It is often unnecessary to obtain (shared) ownership of OpenSSL objects
in this code, and it generally is more costly to do so as opposed to
just obtaining a pointer to the respective OpenSSL object. Therefore,
this patch replaces various OpenSSL function calls that take ownership
with ones that do not.

Refs: #53436
PR-URL: #53460
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
marco-ippolito pushed a commit that referenced this pull request Aug 19, 2024
@tniessen @marco-ippolito
crypto: avoid taking ownership of OpenSSL objects
ceb1d5e 
It is often unnecessary to obtain (shared) ownership of OpenSSL objects
in this code, and it generally is more costly to do so as opposed to
just obtaining a pointer to the respective OpenSSL object. Therefore,
this patch replaces various OpenSSL function calls that take ownership
with ones that do not.

Refs: #53436
PR-URL: #53460
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anonrig anonrig anonrig approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. openssl Issues and PRs related to the OpenSSL dependency. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tniessen @nodejs-github-bot @anonrig @jasnell